Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(build):updated config files #764

Merged
merged 1 commit into from
Jul 10, 2023
Merged

Conversation

vishnusomank
Copy link
Contributor

Purpose of PR?:

To update the config file used to run the discovery engine in both local and helm charts

Fixes #

Does this PR introduce a breaking change?
NO

If the changes in this PR are manually verified, list down the scenarios covered::

Additional information for reviewer? :
Mention if this PR is part of any design or a continuation of previous PRs

Checklist:

  • Bug fix. Fixes #
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • PR Title follows the convention of <type>(<scope>): <subject>
  • Commit has unit tests
  • Commit has integration tests

Copy link
Contributor

@seswarrajan seswarrajan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

deployments/k8s/deployment.yaml Outdated Show resolved Hide resolved
@rksharma95
Copy link
Contributor

rksharma95 commented Jul 5, 2023

i think we should set auto-deploy-dsp to false by default. and another point is that now we are using kustomize to generate the deployment.yaml and any required changes related to the discovery-engine deployment should be made in deployments/k8s/default/discovery-engine and the final deployment yaml can be generated using make manifests.

Copy link
Contributor

@achrefbensaad achrefbensaad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

src/conf/local-file.yaml Outdated Show resolved Hide resolved
@Ankurk99
Copy link
Contributor

Ankurk99 commented Jul 6, 2023

now we are using kustomize to generate the deployment.yaml and any required changes related to the discovery-engine deployment should be made in deployments/k8s/default/discovery-engine and the final deployment yaml can be generated using make generate

@rksharma95 Can we get this documented here?

@rksharma95
Copy link
Contributor

@rksharma95 Can we get this documented here?

@Ankurk99 we already have basic instructions for making changes to deployment, can you please mention here what other information we need to add, thanks

@Ankurk99
Copy link
Contributor

Ankurk99 commented Jul 6, 2023

The automated way of generating the deployment manifest.
make generate

@vishnusomank
Copy link
Contributor Author

The automated way of generating the deployment manifest. make generate

I am getting this error when trying make generate from discovery-engine/deployments/k8s

make generate                                                                                                   (adm-fix|✔)
make: *** No rule to make target 'generate'.  Stop.

@seswarrajan
Copy link
Contributor

The automated way of generating the deployment manifest. make generate

I am getting this error when trying make generate from discovery-engine/deployments/k8s

make generate                                                                                                   (adm-fix|✔)
make: *** No rule to make target 'generate'.  Stop.

@vishnusomank You can just do a make inside discovery-engine/deployments/k8s
It is displaying as text format. Redirect and store it to a file.

@rksharma95
Copy link
Contributor

I am getting this error when trying make generate from discovery-engine/deployments/k8s

my bad it's make manifests i mistakenly typed it as make generate in one of my previous comments. please try with make manifests.

and can you please set auto-deploy-dsp to false for local configs too. thanks @vishnusomank

Copy link
Contributor

@seswarrajan seswarrajan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Signed-off-by: Vishnu Soman <[email protected]>
Copy link
Contributor

@Ankurk99 Ankurk99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@Ankurk99 Ankurk99 merged commit 527705c into accuknox:dev Jul 10, 2023
3 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants